Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Fully configurable non dry-matchers endpoint #7

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rpbaltazar
Copy link

As discussed in other PRs I'm approaching the problem in what I think to be a simpler way.
Some assumptions:

  1. The controller would still be responsible to decide what to do with the data. The endpoint is used mainly to evaluate the operation result and prepare the data for the controller accordingly. The main reason is that different projects can use different representers and different controllers might require that either render a view or data. I think it would be too much to put such responsibility on top of the endpoint.
  2. The proposed endpoint is fully flexible. The developer can overwrite anything on a controller level or on a global level. The developer can also overwrite only the rule or the resolving procs.

This is obviously a draft as there are a few details that I'd like to review, namely:

  1. what to do if there is no matching situation? In my opinion it should fallback in a default matcher that would return unprocessable_entity and a clear message so the developer can more easily spot the problem.
  2. Currently I'm assuming that the operation has a representer class defined or that the representer is passed. What to do in a situation where the resolver needs a representer and it is not present? Since all the projects I'm currently working on are API only, then for me would be fine just try to call to_json on the object, but I'm not sure that would be such a good idea. Any opinions?

@andriimosin
Copy link

It's been a while, but I finally have time to get back and help you with this.

That's really a great job!!! Thank you!!!

Regarding your concerns/thoughts:

  1. Sounds reasonable. It should definitelly going somewhere. In my opinion your fallback case looks good for now
  2. I'm also working on API-only projects and not sure about possible issues with missing representer. Maybe it could be configurable option and in case where having a representer is mandatory, it could throw exception, while in other case it could be to_json as you mentioned. I think we need @apotonick 's opinion here

P.S. I'm ready to help with this and I still want the idea of this gem to come true

@rpbaltazar
Copy link
Author

Hey @andriymosin I think the first step for me would be clean up this. Since this was in a dormant state, I have worked on an endpoint solution that works for our projects. It has had slight fixes here and there depending on the scenarios that we face.

I'll take a look to see how much it differs from this current proposal and update it accordingly.

I spoke with @apotonick a few months ago about this and I think we might need to take a look at how TRB 2.1 would break or not this approach.

@rpbaltazar
Copy link
Author

based on the recent chat, I've updated the endpoint code to my latest version in production.
This uses TRB 2.1 as the strings have been refactored to symbols in TRB.

e.g. result["model"] became result[:model]

This also introduces a concept that we found useful that is sort of an extender for the Result object of an Operation which allows us to know which error type was thrown and adds the method errors that returns the errors accordingly. Please note that for our use case, we mostly fail fast so the scenario of having multiple error types is not handled with this extender. That feature should be easy to add if needed.

If you have any suggestions or discussion moving forward I'm happy to participate on it and give my 2 cts

@rpbaltazar
Copy link
Author

Another thing that its worth mentioning is that our original approach was to have the representer set on the operation level. We moved away from it because we had scenarios where it was useful for us to have a different representer for the same operation result, depending for example if we were replying to a mobile request or a web request.
Because of that, we made the representer part of the params passed. This would need to be updated on the readme as well, but for the moment, I don't think its worth to spend time re-writing the readme before the final approach is defined.

@apotonick
Copy link
Member

apotonick commented Aug 6, 2019

Because of that, we made the representer part of the params passed. This would need to be updated on the readme as well, but for the moment, I don't think its worth to spend time re-writing the readme before the final approach is defined.

That's pretty slick! I've seen other TRB users do the same. I guess configuration should happen on the endpoint level, and the "real" OP is the business logic implementation. In my first drafts, the OP did everything and that's why you could define representers on the class level.

Happy to discuss this!

def call(result, handlers=nil, &block)
matcher.(result, &block) and return if block_given? # evaluate user blocks first.
matcher.(result, &handlers) # then, generic Rails handlers in controller context.
def call(options, overrides)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In examples and tests you have one more argument

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the tests are not yet updated to match the final implementation. They were following my mid way solution. There is definitely a little bit of work to be done here for the tests and readme. Was trying to get the general feeling for the actual solution.

@Tab10id
Copy link

Tab10id commented Aug 6, 2019

From chat:
I think you have same problem as in my solution. For example: If you override default rule or matcher for fallback case contract checks will not called because fallback check was earlier.

@rpbaltazar
Copy link
Author

Nice catch @Tab10id. Once again, this was the solution we built for ourselves and there has not been a reason for overriding the fallback. Fallback for us means we can't find any other way out so we need to reply with something. Above all we wanted to have clear API rules to avoid developer preferences (e.g some people return after a create 200, some other 201 depending on the interpretation of the request)

Extracting the chat to this discussion as it makes more sense than in a general chat with everyone.
@apotonick commented:

I have several ideas and points for endp(o)int. I have my policy chain in the endpoint as well. I guess we could really make it an actual new layer
Couldn't the endpoint be an activity as well, so you can re-wire if you need?

I actually like that idea of making it an activity. Fully agree that it would solve the issue mentioned by @Tab10id

@rpbaltazar
Copy link
Author

Trying to not let this one die again, I've extracted the concept to an endpoint activity based. I'm not fully familiar yet with the Activity DSL but I think something along these lines would work.

https://github.com/rpbaltazar/trailblazer-endpoint/blob/endpoint-proposal-activity/lib/trailblazer/endpoint_activity.rb

I'll try to push it to a fully working state and when I reach that point, open the PR with this version. if you want to take a look in the meanwhile and give some opinions, please go ahead.

@differencialx
Copy link

If somebody are looking for more flexible alternative of trailblazer endpoint, please look at https://github.com/differencialx/simple_endpoint

@rpbaltazar
Copy link
Author

@apotonick i've been a bit absent, but i know you guys have been working/discussing on it.

@differencialx are you currently using this in a production env?

@differencialx
Copy link

@rpbaltazar yes, it's used by several projects

@apotonick
Copy link
Member

apotonick commented May 26, 2020

This couldn't have come at a better time @differencialx - I'm about to finish the new endpoint implementation and was thinking about the public API to be used in controllers. Some parts are already very similar to yours, some I might have to steal 😬 I would love to hear your feedback, and maybe "porting" your implementation to work with the official one shouldn't be too hard. 🍻

@apotonick
Copy link
Member

@differencialx
Copy link

@apotonick I would be very happy if you would use my implementation to improve trailblazer endpoint. So I don't mind. 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants